Fix device cleanup to run on device disable#16
Conversation
The cleanup callback was incorrectly registered on the driver object instead of the device object. This meant that when disabling the device in Device Manager, FrameworkArgbEvtDriverContextCleanup was never called (it only runs on driver unload, not device disable). Additionally, the driver cleanup was calling GetDeviceContext() on a WDFDRIVER object, which is incorrect - that function is for WDFDEVICE. Changes: - Add FrameworkArgbEvtDeviceCleanup callback for device-level cleanup - Register it on the device attributes in FrameworkArgbCreateDevice - Move EC handle cleanup from driver to device cleanup - Keep only WPP tracing cleanup in driver cleanup This fixes issues when disabling the device while Windows Dynamic Lighting is using it (timer keeps firing, reboot popup, etc). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
JohnAZoidberg
force-pushed
the
fix-disabling
branch
from
d486d52 to
2bb7505
Compare
|
Disabling the device now works:
I will run the HLK tests to confirm everything is ok |
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug where device cleanup was not being executed when the device was disabled in Device Manager, only when the driver was unloaded. The cleanup callback was incorrectly registered at the driver level instead of the device level, and was using the wrong API to get the device context.
Changes:
- Moved EC handle cleanup from driver-level to device-level cleanup callback
- Added
FrameworkArgbEvtDeviceCleanupcallback registered on device attributes - Corrected driver cleanup to only handle WPP tracing termination
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| FrameworkArgb/Driver.c | Removed EC handle cleanup logic from driver cleanup; now only handles WPP tracing |
| FrameworkArgb/Device.h | Added declaration for new device cleanup callback |
| FrameworkArgb/Device.c | Registered device cleanup callback and implemented EC handle cleanup at device level |
Comments suppressed due to low confidence (1)
FrameworkArgb/Driver.c:1
- The error message states 'Failed to close CrosEc Handle' but this was logged after successfully closing the handle. This misleading error message has been removed in the updated code.
/*++
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CloseHandle(DeviceContext->CrosEcHandle); | ||
| DeviceContext->CrosEcHandle = INVALID_HANDLE_VALUE; | ||
| TraceInformation("%!FUNC! Closed CrosEc Handle"); |
There was a problem hiding this comment.
Setting the handle to INVALID_HANDLE_VALUE after closing is good practice to prevent double-close bugs, but this happens after the close operation. If CloseHandle() fails, the handle should not be set to INVALID_HANDLE_VALUE. Consider checking the return value of CloseHandle() and only setting to INVALID_HANDLE_VALUE on success.
| CloseHandle(DeviceContext->CrosEcHandle); | |
| DeviceContext->CrosEcHandle = INVALID_HANDLE_VALUE; | |
| TraceInformation("%!FUNC! Closed CrosEc Handle"); | |
| BOOL closeResult = CloseHandle(DeviceContext->CrosEcHandle); | |
| if (!closeResult) { | |
| TraceError("%!FUNC! Failed to close CrosEc Handle, error: %lu", GetLastError()); | |
| } else { | |
| DeviceContext->CrosEcHandle = INVALID_HANDLE_VALUE; | |
| TraceInformation("%!FUNC! Closed CrosEc Handle"); | |
| } |
There was a problem hiding this comment.
Hmmm, I wonder in what cases CloseHandle fails and if in those cases we want to avoid seetting the handle to invalid.
I would guess that in those cases the handle is actually already invalid or corrupted, so it may actually be good to still set it to INVALID_HANDLE_VALUE.
|
I ran the HLK tests that previously failed and now they succeed 🤯 |
The cleanup callback was incorrectly registered on the driver object instead of the device object. This meant that when disabling the device in Device Manager, FrameworkArgbEvtDriverContextCleanup was never called (it only runs on driver unload, not device disable).
Additionally, the driver cleanup was calling GetDeviceContext() on a WDFDRIVER object, which is incorrect - that function is for WDFDEVICE.
Changes:
This fixes issues when disabling the device while Windows Dynamic Lighting is using it (timer keeps firing, reboot popup, etc).
Resolves #5 and resolves #7